Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat : Add Phone Number Module : [ATLAS-24] #1

Merged
merged 38 commits into from
Dec 6, 2023
Merged

feat : Add Phone Number Module : [ATLAS-24] #1

merged 38 commits into from
Dec 6, 2023

Conversation

RgnDunes
Copy link
Collaborator

@RgnDunes RgnDunes commented Oct 26, 2023 •
edited
Loading

Description

This PR adds Phone Number module to the library.

Fixes # (issue)

Additional snippets: [Thread](Paste the thread link here....)

Screenshot 2023-12-06 at 5 15 53 PM

Jira/Asana

JIRA : https://razorpay.atlassian.net/browse/ATLAS-24

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Local
  • Test B

Checklist:

  • Add Jira ID(s) in PR title and in the description?
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added positive and negative tests that prove my fix is effective or that my feature works - PR
  • New and existing unit tests pass locally with my changes
  • Responsive design is handled if there are new components/UI changes?
  • Any Screenshots (mobile & desktop) required for PR? If yes, have you added the respective screenshots ?
  • Any manual dev testing done by you on beta/func environment?
  • Reviewer added (SLA 2 days)

Reviewer Checklist

  • PR has right logic changes and functionality with tests is implemented as expected - Confirmed by @tarun-khanna

@RgnDunes RgnDunes changed the title add phone number module code feat : Add Phone Number Module : Oct 26, 2023
@RgnDunes RgnDunes changed the title feat : Add Phone Number Module : feat : Add Phone Number Module : ATLAS-24 Oct 26, 2023
@RgnDunes RgnDunes changed the title feat : Add Phone Number Module : ATLAS-24 feat : Add Phone Number Module : [ATLAS-24] Oct 26, 2023
@RgnDunes RgnDunes requested a review from tarun-khanna October 30, 2023 12:51
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@rzpcibot
Copy link
Contributor

rzpcibot commented Nov 10, 2023 •
edited
Loading

Warnings
⚠️ Your PR has over 600 lines of code additions 😱 . Try to breakup into separate PRs 👍

Bundle Size Report

Files

click to expand/collapse
🟢 No Change 🗑 File Deleted 🆕 New File 📈 Size Increased 👍 Size Reduced
Parsed (kb)
🚦 File Name Base PR Diff %
📈 cjs/index.js 12.02 32.97 $\textcolor{red}{20.95}$ 174.29
📈 esm/index.js 11.98 32.94 $\textcolor{red}{20.96}$ 174.96
📈 umd/index.js 13.57 37.17 $\textcolor{red}{23.6}$ 173.91

Generated by 🚫 dangerJS against a3af1fd

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6a48fb4) 83.16% compared to head (a3af1fd) 90.63%.
Report is 8 commits behind head on master.

Files Patch % Lines
src/modules/phoneNumber/index.ts 0.00% 3 Missing ⚠️
src/modules/phoneNumber/isValidPhoneNumber.ts 93.75% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage   83.16%   90.63%   +7.46%     
==========================================
  Files          13       22       +9     
  Lines         101      235     +134     
  Branches       28       61      +33     
==========================================
+ Hits           84      213     +129     
- Misses         17       22       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tarun-khanna
tarun-khanna suggested changes Nov 28, 2023
View reviewed changes
src/modules/phoneNumber/__tests__/formatPhoneNumber.test.ts Show resolved Hide resolved
src/modules/phoneNumber/__tests__/isValidPhoneNumber.test.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/__tests__/parsePhoneNumber.test.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/__tests__/parsePhoneNumber.test.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/data/phoneFormatterMapper.ts Show resolved Hide resolved
src/modules/phoneNumber/formatPhoneNumber.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/isValidPhoneNumber.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/isValidPhoneNumber.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/parsePhoneNumber.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/utils.ts Outdated Show resolved Hide resolved
tarun-khanna
tarun-khanna reviewed Dec 5, 2023
View reviewed changes
src/modules/phoneNumber/data/dialCodeMapper.ts Outdated
@@ -0,0 +1,245 @@
/* Source: Google LibPhoneNumber Metadata: https://github.com/google/libphonenumber/blob/master/javascript/i18n/phonenumbers/metadata.js */

export const dialCodeMapper: { [key: number]: string[] } = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this getting used ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it is getting used 😆 in detectCountryCodeFromDialCode .

@tarun-khanna
Copy link
Contributor

IMO throw errors when the APIs do not match the schema, otherwise silently return the input as it is.
This will prevent breaking the flow experience for users.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 5, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: a3af1fd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

Unit Test Results

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌

Results for commit 937107a.

@RgnDunes RgnDunes requested a review from tarun-khanna December 5, 2023 12:47
tarun-khanna
tarun-khanna suggested changes Dec 6, 2023
View reviewed changes
src/modules/phoneNumber/__tests__/isValidPhoneNumber.test.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/__tests__/parsePhoneNumber.test.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/data/dialCodeMapper.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/utils.ts Show resolved Hide resolved
src/modules/phoneNumber/utils.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/utils.ts Outdated Show resolved Hide resolved
src/modules/phoneNumber/parsePhoneNumber.ts Show resolved Hide resolved
@RgnDunes RgnDunes merged commit 7c85b16 into master Dec 6, 2023
11 checks passed
@RgnDunes RgnDunes deleted the phone-number-module branch December 6, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarun-khanna tarun-khanna tarun-khanna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RgnDunes @rzpcibot @codecov-commenter @tarun-khanna